-
Notifications
You must be signed in to change notification settings - Fork 125
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update to DynamicSumTypes 3 #1055
Conversation
What?! You need to provide much more information for such a huge change. This is one of the big new things in v6 and you want to deprecate it in 6.1. This is not good experience for a user. |
I don't understand this PR at all. |
This is still totally incomplete, I will add a lot more info when I finish the transition. In practice though we can replace all the work of using Agents, DynamicSumTypes
# how many agents you want
@agent struct FirstAgent end
@agent struct SecondAgent end
# the sum type which removes all type instabilities available in DynamicSumTypes
@sumtype BothAgent(FirstAgent, SecondAgent) <: AbstractAgent No need for a special macro for dispatch, no need to if-else branches. The sum type is actually made of real agents, and not by opaque representation like in |
If you take a brief look at the ReadMe repo you will see how simple the new methodology is: https://github.com/JuliaDynamics/DynamicSumTypes.jl |
okay, when you update this please also write out, at least in brief, what was the new "magic" you discovered that made this possible at an internal level, as I am not sure I can figure it out by source diving. |
In practice the "magic" is just to enclose all the types in a single one and then use if-else branches to type-stabilize the result of the various functions one needs to operate on them. I somehow overlooked this simple methodology initially because I didn't expect that something this simple could work that well considering the complexity of other packages trying to do something similar. I tested the methodology from 1.9 on with our benchmarks and it always removes any dynamic dispatch there was with a There is actually no need to go further than this in my opinion, Julia is good enough to do all the rest for us 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am stopping the review after reading through the tutorial. I've added lots of comments. Unfortunately @Tortar, I don't see this as an upgrade of what we had before. I don't see it as a downgrade either.
Please try to explain how this is a better version of what was happening with the @multiagent
. Please keep in mind: we dont care how complicated the source code of DynamicSumTypes.jl is. We care about how complicated it is to use Agents.jl. My point being that this is a PR into Agents.jl, which is a rather front-end package. So the value of PRs here is mainly value for an end-user. That is why having e.g. sorter source code in DynamicSumTypes.jl is not an argument in favour of this PR by itself.
To my understanding, the main advantage over using @multiagent
is that 1) @multiagent
can lead to a rather large chunk of code, because all subagent types must be inside, while with this new approach the agent types are still kept separately. More over in this approach existing agent types can be reused. However, is this really such a big advantage? Agent types can only defined once regardless, as Julia can't redefine types. So does it matter if the agent declaration is inside @multiagent
or not? With this new approach, you can also either use MultiAgent
or Union
. But does this really matter? As far as I can tell, one almost never has a reason to use Union
, as the multi agent infrastructure is so good.
Now, here are some points where I don't see any advantage:
- We replaced
@multiagent
with creating the other wrapper typeMultiAgent
. So we don't really gain anything here. - We didn't get rid of needing a replacement for
typeof
. We went fromkindof
totypeof(variant
. This is not an improvement and I would argue it is more complex overkindof
. - We didn't get rid of now having to alter the "standard Julia multiple dispatch" for the agent stepping function. We went from the
@dispatch
usage to the 3-argumentagent_step!
function, where one of the three input arguments isn't even used anywhere in the function. It is just used for dispatch. In my eyes this is a downgrade, because it is increase in complexity, without any obvious benefit. I like more the@dispatch
and the 2-argument agent step, because it is more harmonious with the rest of the library. - We have a clear loss of benefit as now you can't use
add_agent!
the typical way. In 99% of the cases I've used Agents.jl, I was adding agents to the model via theadd_agent!
automatic route, I wasn't first creating them and then adding them to the model.
If you think this new approach is a better route for Agents.jl, then please make it transparent why you think so. I am really sorry for the negative review, as you've put in some work on this. But it is important that such a big change overcomes sufficient adversity before getting in.
Main comment 2: it is totally fine for you to reference DynamicSumTypes.jl, but the typical user of Agents.jl does not need to hear about this package, or even using
it in their code. Instead, you can mention this infrastructure at the end of the docstring of @sumtype
or the wrapper macro for it with a more Agents.jl like name.
Right, but did you test if there is any performance regression with respect to using the current |
thanks for the review @Datseris, it is indeed a big change, but as I will explain really worthwhile in my opinion. The problematic aspects of
In practice the new approach is really the same of a From the performance standspoint is actually amazing in my opinion. It is faster and more memory efficient than |
I think the tutorial should say how to add agents of a certain type in the model via |
Thanks for finishing this @Tortar . Fine we can leave things as they are for the 3 argument version. I personally like the 2 argument only existing but oh well. Let's try to get this in soon as I've seen several people use |
Co-authored-by: George Datseris <[email protected]>
yes let's hope that this is the case, unfortunately this was a bit of unknown territory for me so I made some overly complex implementation at the start :-) I addressed your review, if you approve I will do a final check and then merge |
@multiagent
Co-authored-by: George Datseris <[email protected]>
Co-authored-by: George Datseris <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are almost there, really. Only minor stuff and the event que model!
Damn, using the multiagent instead of Union makes the event que example significantly more complicated. |
not too much in my opinion, it is also a bit faster both in 1.10 and 1.11, 500ns per event instead of 560, not much but something :D |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WIll merge when docs build.
DynamicSumTypes v3 makes it really really easy to work with sum types directly, with much more flexibility than before and a much cleaner approach, we need to deprecate
@multiagent
because it was backed by the previous approach. This time I think we can go instead by integration, listing DynamicSumTypes in the "Performance Tips" of the manual and giving an example because we actually need almost no work here for that.This will also fix #1039